Replace _has<PlotType> variables with '_has()' fullLayout method #491

etpinard · 2016-04-28T18:59:44Z

Rationale

Our set of hidden plot type variables _hasCartesian, _hasGL3D, _hasGeo, _hasGL2D, _hasPie and most recently _hasTernary has grown out-of-control.

We need a better way to keep track of the plot types present on a graph AND loop over the required plot modules. Enter the fullLayout._basePlotModules array and the fullLayout._has method.

In brief

This PR:

Makes Plotly.plot is agnostic to base plot modules. Consequently, users can now write their own basePlotModule.
Moves the list of trace modules required for a given graph from gd._modules to fullLayout._modules (it is much easier to work with from fullLayout)
Keeps track of the plot modules required for a given graph in fullLayout._basePlotModules
Make pie traces use their own basePlotModule instead of being a special case in the Cartersian base plot module
Attaches a _has method to fullLayout to check the presence of a particular plot type e.g. fullLayout._has('gl3d') return true if the given graph has a gl3d subplot. The _has method deprecates the _hasCartesian, _hasGeo ... fullLayout fields.
All _hasCartesian, _hasGeo ... fields have been replaced by their _has('') equivalent
Important, _hasCartesian, _hasGeo ... are still defined for backward compatibility as some apps (e.g. the old plot.ly workspace) uses them.

Going forward, patterns that require checking for which plot types are present on a given graph should be discouraged. All plot types should have compatible components meaning that a loop over the present basePlotModules should suffice in all cases.

In this regard, the most problematic component at the moment is the modebar. In a future PR, we should try to make the modebar a component of each plot type e.g. Cartesian.modeBar, Geo.modeBar ... etc.

- as a replacement for _hasCartesian, _hasGeo, _hadGL3D, etc.

- revert after replacing all _has with _has()

- so that fullLayout.has('pie') returns true - note that fullLayout.has('cartesian') also returns true now as pie trace use the cartesian plot module.

- so that 'hovermode' is set and toggled properly.

etpinard · 2016-04-28T19:08:21Z

src/plots/plots.js

    }

+    // attach helper method
+    newFullLayout._has = hasPlotType.bind(newFullLayout);


@mdtusz I know this could be more verbose, but I really like the look of e.g. fullLayout._has('geo')

alexcjohnson · 2016-04-29T08:18:44Z

💯 for the idea. It's going to be a bit slower than all the individual flags but doesn't look like it's called much in hot paths, so I think we're fine. I wonder if we can even turn this block into a loop over fullLayout._modules - or actually, that needs to be a loop over basePlotModules, which makes me wonder if we shouldn't make a fullLayout._basePlotModules too?

etpinard · 2016-04-29T14:25:46Z

I wonder if we can even turn this block into a loop over fullLayout._modules - or actually, that needs to be a loop over basePlotModules, which makes me wonder if we shouldn't make a fullLayout._basePlotModules too?

That's the goal. But, I wanted to leave that for another PR. Replacing all those _has<PlotType> variable will turn into a lot of changed lines.

I did experiment with a fullLayout._basePlotModules list, it works great except for the orphan axes case, where we would have to make sure that Catersian.plot gets calls even if there's no 'cartesian' trace on the graph.

To note, a loop over fullLayout._basePlotModules list would users to write their own pluggable basePlotModule. For example, our Mapbox integration could be simply plotly.js plugin instead of being part of this repo.

alexcjohnson · 2016-04-29T16:25:21Z

To note, a loop over fullLayout._basePlotModules list would users to write their own pluggable basePlotModule.

Sure... Though it's better for the ecosystem if those modules eventually make it into the main repo - so users can still share plots, including via plot.ly.

This reverts commit e63ba4a.

- so that _has??? are one-to-one with base plot modules - handle case where base plot module attrRegex isn't defined in plot schema.

- useful to fill fullLayout._modules and fullLayout._basePlotModules

- and in _basePlotModules in fullLayout._has() instead of loop over _modules

- in Plots.cleanPlot (use basePlotModules list in old fullLayout) - in Plots.supplyLayoutModulesDefaults (making those !_has??? obsolete) - in main drawData function (instead of _has??? switch block)

- so that Cartesian.plot gets called

- making for one less _has call

- if found, the restyle calls required a full replot

- Plots.redrawText and Snapshot redraw work with every plot type except polar - continue if polar trace is detected in main supply default trace loop - don't assign _hasPolar (it didn't have to correct value anyway)

- no need to loop over all the registered module, only the ones that appear in the data suffices.

- so that it works in cases where _has isn't defined e.g. via Plotly.purge().

- to more easily mock _has in test layouts

- so that orphan axes are considered hasCartesian still - IMPORTANT: we must keep ref to _has??? for backward compatibility

etpinard · 2016-05-06T20:08:06Z

@alexcjohnson @mdtusz I decided to through with making Plotly.plot agnostic to basePlotModules in this PR.

I completely re-wrote the PR description. This PR is quite massive, but the commits history starting from May 6 should be easy to read.

etpinard · 2016-05-06T20:11:49Z

src/plot_api/plot_api.js

+    // to not go through a full replot
+    var doPlotWhiteList = ['cartesian', 'pie', 'ternary'];
+    fullLayout._basePlotModules.forEach(function(_module) {
+        if(doPlotWhiteList.indexOf(_module.name) === -1) doplot = true;


In restyle, which is more general than

if(fullLayout._hasGL3D || fullLayout._hasGeo || fullLayout._hasGL2D) doplot = true;

in an effort to support custom basePlotModules

mdtusz · 2016-05-09T21:32:13Z

Don't see anything wrong here. We'll have to remember to swap that block of fullLayout._has???'s out once the workspace doesn't need them anymore, but looks good to me (once tests pass)!

💃

etpinard · 2016-05-11T18:05:57Z

src/plots/cartesian/layout_defaults.js

@@ -95,7 +95,7 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
    // make sure that plots with orphan cartesian axes
    // are considered 'cartesian'
    if(xaListCartesian.length && yaListCartesian.length) {
-        layoutOut._hasCartesian = true;
+        Lib.pushUnique(layoutOut._basePlotModules, Plots.subplotsRegistry.cartesian);


Important ⏫

etpinard added 7 commits April 27, 2016 18:14

mv gd._modules -> fullLayout._modules

88f552a

lint

525e3c7

attach has (plot type) method to fullLayout,

876ce4b

- as a replacement for _hasCartesian, _hasGeo, _hadGL3D, etc.

add tempory block _has() -> _hasCartesian etc.

f45ffec

- revert after replacing all _has with _has()

rm _has??? attributes

4e0c6da

make 'pie' a layout category,

93493a6

- so that fullLayout.has('pie') returns true - note that fullLayout.has('cartesian') also returns true now as pie trace use the cartesian plot module.

check for hasCartesian && !hasPie for 'hovermode',

e63ba4a

- so that 'hovermode' is set and toggled properly.

etpinard added type: maintenance labels Apr 28, 2016

etpinard reviewed Apr 28, 2016
View reviewed changes

etpinard added 3 commits May 3, 2016 14:25

Revert "check for hasCartesian && !hasPie for 'hovermode',"

b655cb0

This reverts commit e63ba4a.

split pie trace base plot module from cartesian:

f476e05

- so that _has??? are one-to-one with base plot modules - handle case where base plot module attrRegex isn't defined in plot schema.

rm (now useless) trace module layoutCategories

1c41c2e

etpinard mentioned this pull request May 5, 2016

Fix relayout axis category #510

Merged

etpinard added 9 commits May 6, 2016 14:04

add fill unique lib function,

313150e

- useful to fill fullLayout._modules and fullLayout._basePlotModules

fill fullLayout._basePlotModules list in default trace loop,

392619c

- and in _basePlotModules in fullLayout._has() instead of loop over _modules

loop over basePlotModules instead of subplotRegistry in:

3787d47

- in Plots.cleanPlot (use basePlotModules list in old fullLayout) - in Plots.supplyLayoutModulesDefaults (making those !_has??? obsolete) - in main drawData function (instead of _has??? switch block)

make sure to add cartesian base plot module to orphan axis graphs:

68b12c4

- so that Cartesian.plot gets called

move 3d axes init step from makePlotFrameork -> Gl3d.plot

a41427c

- making for one less _has call

check for non-cartesian/pie/ternary base plot module in restyle,

a48d071

- if found, the restyle calls required a full replot

handle a few polar-only cases:

a78521c

- Plots.redrawText and Snapshot redraw work with every plot type except polar - continue if polar trace is detected in main supply default trace loop - don't assign _hasPolar (it didn't have to correct value anyway)

sub has??? -> has('???')

aa2ec8b

loop over (trace) modules in trace supplyLayoutDefaults:

77fb7e2

- no need to loop over all the registered module, only the ones that appear in the data suffices.

etpinard added 5 commits May 6, 2016 14:38

robustify pie clean step:

a141d78

- so that it works in cases where _has isn't defined e.g. via Plotly.purge().

put ref of hasPlotType method on Plots:

dc297ca

- to more easily mock _has in test layouts

move block of _has?? = _has('') after all defaults are filled:

e3f72f0

- so that orphan axes are considered hasCartesian still - IMPORTANT: we must keep ref to _has??? for backward compatibility

update tests

c59a960

add info about data/layout and fullData/fullLayout

9f94494

etpinard added status: reviewable and removed status: in progress labels May 6, 2016

Merge branch 'master' into has-plot-type

9837d8b

etpinard reviewed May 6, 2016
View reviewed changes

etpinard added 2 commits May 9, 2016 17:16

rename Lib.fillUnique -> Lib.pushUnique

0c7b2f8

rm unnecessary boolean type coercion

5e6759b

etpinard reviewed May 11, 2016
View reviewed changes

etpinard merged commit 1903061 into master May 11, 2016

etpinard deleted the has-plot-type branch May 11, 2016 18:16

etpinard mentioned this pull request May 19, 2016

Seamless subplots #554

Merged

etpinard mentioned this pull request Jun 10, 2016

Introducing plotly.js + Mapbox GL #626

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace _has<PlotType> variables with '_has()' fullLayout method #491

Replace _has<PlotType> variables with '_has()' fullLayout method #491

etpinard commented Apr 28, 2016 •

edited

Loading

etpinard Apr 28, 2016

alexcjohnson commented Apr 29, 2016

etpinard commented Apr 29, 2016

alexcjohnson commented Apr 29, 2016

etpinard commented May 6, 2016

etpinard May 6, 2016

mdtusz commented May 9, 2016

etpinard May 11, 2016

Replace _has<PlotType> variables with '_has()' fullLayout method #491

Replace _has<PlotType> variables with '_has()' fullLayout method #491

Conversation

etpinard commented Apr 28, 2016 • edited Loading

Rationale

In brief

etpinard Apr 28, 2016

Choose a reason for hiding this comment

alexcjohnson commented Apr 29, 2016

etpinard commented Apr 29, 2016

alexcjohnson commented Apr 29, 2016

etpinard commented May 6, 2016

etpinard May 6, 2016

Choose a reason for hiding this comment

mdtusz commented May 9, 2016

etpinard May 11, 2016

Choose a reason for hiding this comment

etpinard commented Apr 28, 2016 •

edited

Loading